Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add all_cars_in_dragway python demo; Remove C++ automotive_demo #396

Merged
merged 3 commits into from
May 10, 2018
Merged

Add all_cars_in_dragway python demo; Remove C++ automotive_demo #396

merged 3 commits into from
May 10, 2018

Conversation

apojomovsky
Copy link

Fixes the first item of the first list in #378

  • Removes the demo_launcher.py as well as the automotive_demo.cc.
  • Adds the all_cars_in_dragway python demo, with the same basic functionality as the pair demo_launcher + automotive_demo.

Missing:

  • A trajectory-car for the demo. We might need to bind or at least mimic the functionality found in drake's CreateTrajectoryParams function, in order to generate the parameters for the car.

@basicNew
Copy link

basicNew commented May 8, 2018

@stonier just to double check: last time we spoke about python bindings I recall we decided not to work on the trajectory car, as that was (is?) undergoing a refactoring in drake. As we port this demo from C++ to port we would remove the last demo that showed that car. Is that ok? Want me to add an issue for milestone 3 to bring it back?

@apojomovsky apojomovsky force-pushed the apojomovsky/all_cars_in_dragway_demo branch from 08fbb62 to ac2e16b Compare May 8, 2018 17:15
basicNew
basicNew reviewed May 8, 2018
View reviewed changes
Copy link

@basicNew basicNew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @apojomovsky , just left a comment about refactoring car-creation functions.

backend/python/examples/all_cars_in_dragway.py Outdated

SIMULATION_TIME_STEP_SECS = 0.001

def add_simple_car(simulator):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is not specific of this issue, but seems like a good time to do it. Could you please move this functions to simulation_utils.py and make them parametrizable? Then you could use them in loadable_agent_simulation.py and railcar_in_multilane.py (as well as refactoring the build_simple_car_simulator in simulation_utils.py).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good! done!

hidmic
hidmic suggested changes May 8, 2018
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple nit-picks. Overall looks great!

backend/python/examples/all_cars_in_dragway.py Outdated
SIMULATION_TIME_STEP_SECS = 0.001

def add_simple_car(simulator):
"""Instanciates a new Simple Prius Car
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apojomovsky here and below, typo instanciates -> instantiates.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, thanks! done!

backend/python/examples/all_cars_in_dragway.py Outdated
"lane_direction": Any(lane_direction),
"start_params": Any(start_params)
}
# Instantiate a Loadable Rail Car.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apojomovsky Instantiate -> Instantiates

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for that, fixed!

@apojomovsky apojomovsky force-pushed the apojomovsky/all_cars_in_dragway_demo branch 3 times, most recently from b282b89 to 22a3b07 Compare May 8, 2018 20:21
@apojomovsky
Copy link
Author

Thanks for the review @basicNew and @hidmic , I just finished addressing your comments. Could you PTAL?

@stonier
Copy link
Collaborator

stonier commented May 9, 2018 •
edited by apojomovsky
Loading

Some minor things:

  • get the generate_python_docs.sh to populate items in alphabetical order
  • do we still need loadable_agent_simulation.py? The all_cars_in_dragway` should provide demo coverage (for the cars at least, should still have something for non-road, dragway, multilane variations)
  • fix conflicts from recent merge of [infra] python un-nested #391
  • update python_examples.md.template.in to point to the delphyne guide for installation instructions
  • update the delphyne guide to point to the all_cars_in_dragway instead of the demo_launcher as the defacto example

The add_xyz() scripts are very useful! I'll let @hidmic confirm the review.

@stonier
Copy link
Collaborator

stonier commented May 9, 2018

@stonier just to double check: last time we spoke about python bindings I recall we decided not to work on the trajectory car, as that was (is?) undergoing a refactoring in drake. As we port this demo from C++ to port we would remove the last demo that showed that car. Is that ok? Want me to add an issue for milestone 3 to bring it back?

Yes, a new trajectory agent just went into Drake. Mostly RobotLocomotion/drake#8720 and RobotLocomotion/drake#8725. I'll probably play with them as I do this 'has-a' relationship between delphyne loadable wrappers and Drake agents.

@basicNew
Copy link

basicNew commented May 9, 2018

Ack, thanks for the heads-up @stonier on new trajectory agents in Drake.

@apojomovsky apojomovsky force-pushed the apojomovsky/all_cars_in_dragway_demo branch from 22a3b07 to 33d7af9 Compare May 9, 2018 16:58
@apojomovsky
Copy link
Author

apojomovsky commented May 9, 2018 •
edited
Loading

Thanks for you feedback @stonier ! I just finished addressing your comments.
Regarding the loadable_agent_simulation.py, you're right that it's not needed anymore, so I just removed it. As per multilane road, and since we already have the railcar_in_multilane.py around, do you think we should add another demo?

@stonier
Copy link
Collaborator

stonier commented May 9, 2018

Should at least have something that tickles the possibility of loading without roads (~ loadable_agents_demo with simple car previously). If that's just in as a test, that's also fine - i.e. doesn't have to be a demo.

@apojomovsky
Copy link
Author

apojomovsky commented May 9, 2018 •
edited
Loading

There are a couple of demos that already exercises this no-road functionality: time_bounded_simulation, start_simulation_paused, keyboard_controlled_demo, among others. Adding this as an option for this demo would involve having only a simple car on stage, so I'd chose to leave this particular demo as is. What do you think @stonier ?

@stonier
Copy link
Collaborator

stonier commented May 10, 2018

Agreed

@apojomovsky apojomovsky merged commit 2bb57d1 into master May 10, 2018
@apojomovsky apojomovsky deleted the apojomovsky/all_cars_in_dragway_demo branch May 10, 2018 13:15
@stonier stonier mentioned this pull request May 10, 2018
Closed
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@basicNew basicNew basicNew left review comments

@hidmic hidmic hidmic approved these changes

@clalancette clalancette Awaiting requested review from clalancette

@caguero caguero Awaiting requested review from caguero

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apojomovsky @basicNew @stonier @hidmic